New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(internal/gensupport): add configurable retry #1324
Conversation
Extend SendAndRetryRequest to allow retry options (backoff and ErrorFunc) to be passed in. Add a surface to the generated code for storage to allow the manual layer to choose whether the request is retried. I tested this out via the manual layer by verifying that the ObjectsInsertCall only retries now when WithRetry() is applied. I still need to update the generator to ensure that the changes in the generated file will persist and probably add some tests as well. Thought I'd try and get an initial review first.
Depends on merge and release of googleapis/google-api-go-client#1324
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me, but I'll let @codyoss approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one main question on retries for resumable upload in general
"which errors are considered retryable. By default, exponetial backoff will be" + | ||
"applied using gax defaults, and the following errors are retried:" + | ||
"\n\n" + | ||
"- HTTP responses with codes 429, 502, 503, and 504." + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the error codes be pulled from gax instead of string literals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing as this is just a one-off comment and these values are not likely to change, I don't think it's work adding extra complexity to the templating in order to accomplish this.
@@ -50,7 +52,7 @@ func send(ctx context.Context, client *http.Client, req *http.Request) (*http.Re | |||
// If ctx is non-nil, it calls all hooks, then sends the request with | |||
// req.WithContext, then calls any functions returned by the hooks in | |||
// reverse order. | |||
func SendRequestWithRetry(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) { | |||
func SendRequestWithRetry(ctx context.Context, client *http.Client, req *http.Request, retry *RetryConfig) (*http.Response, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I think will be an issue is that the remote offset is not considered in the case Storage has progressed and the client is now unaligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that work will have to come in a separate PR. This PR doesn't change the mechanics of the resumption strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Depends on merge and release of googleapis/google-api-go-client#1324
Extend SendAndRetryRequest to allow retry options (backoff and
ErrorFunc) to be passed in. Add a surface to the generated code
for storage to allow the manual layer to choose whether the request
is retried.
Idempotency considerations will be handled at the manual layer.
I tested this out via the manual layer by verifying that the
ObjectsInsertCall only retries now when WithRetry() is applied.
I still need to update the generator to ensure that the changes in
the generated file will persist and probably add some tests as well.
Thought I'd try and get an initial review first.